Isn't this problematic for overridden clone_obj that reuse zend_objects_clone_obj? In that case old_object->handlers->clone_obj != zend_objects_clone_obj would hit, and we'd recurse infinitely.

Yes. There are none in php-src and generally speaking it seems unlikely that there are any, since when there is a need to overwrite clone there likely also is a need to overwrite create (which explicitly isn't supported):

/* assume that create isn't overwritten, so when clone depends on the

• overwritten one then it must itself be overwritten */

Most of the custom clone handlers just use zend_objects_clone_members().

This PR tries to minimize the disruption for existing code, but if there is an external extension that does what you suggest, then it's easy enough for the author to adjust it to add an explicit clone_with handler.

Alternatively, you could do the inverse: Keep clone_obj as is, call it whenever properties are not provided or when the array is empty. When it isn't, clone_obj_with is invoked, which can call clone_obj to clone the object, and adjust the properties afterwards. This way, an extension could simply continue to provide clone_obj and the property part would just work. This approach seems more logical to me.

which can call clone_obj to clone the object, and adjust the properties afterwards

This is likely dangerous, since some internal classes might expect their (readonly) properties not to change, which leads to some C structures related to a property not being properly adjusted. Random\Randomizer is currently not cloneable at all, but it stores both a public readonly \Random\Engine $engine property exposed to userland and also the raw “engine pointer” for more efficient access.

The implementation also prefers the clone_with handler, since I would like to see the old clone_obj hook to go away in the future to remove the duplication.

This is likely dangerous, since some internal classes might expect their (readonly) properties not to change

Can this already be broken with __clone in a user-declared sub-class? I can't see cases other than enums preventing the declaration of __clone. The property would need to be private(set) to prevent this.

Can this already be broken with __clone in a user-declared sub-class?

Ah, yes. Implicit protected(set), of course. I've implemented your suggestion in 26ae13b. While it allowed to remove quite a bit of the custom logic, I feel that overall the code is in a worse shape with the removal of “clone with” being a first-class citizen.

Documenting the small break to the internal API that you found would be preferable to me (thus dropping that commit).

What do you think?

Well, the new commit looks much more logical to me, and it should work automatically for extensions that already customize clone. Right now I'm failing to see the downsides, but maybe some others can chime in.

Right now I'm failing to see the downsides,

For me it made sense to have the “clone-with” logic as part of the zend_objects_clone_members to have all the cloning logic in a single place, particularly for classes that do need a custom clone_with and would have to reimplement it with the new commit.

I agree with Ilija on this. I like the current code more.
You could split off the property update logic to a separate helper so that part can be reused? I'd think that either:

• A custom class may want to do some validation and call such a helper
• A custom class has entire custom logic to deal with it

Okay. I've also gotten private feedback that the new version is preferable, so I'll leave it like that. Thanks!

If write_property in the loop below triggers a hook or magic setter, then you can set readonly properties that were not included in withProperties. I don't know if this is desired or not.

Will need to give this some thought / run some tests regarding the current behavior of __clone() with hooks / magics and discuss this with @edorian.

We discussed this and consider this to be desired: A property hook is added by the class itself and thus the class author has full control over what the hook does.

If updating some readonly property as part of the assignment to another property is required to keep an object in a consistent state, then it should still be allowed to update that property with clone-with. Of course trying to also assign that readonly property with a value given in clone-with will then fail, but this is consistent with regular assignments when the readonly property is still uninitialized.

I've added Zend/tests/clone/clone_with_012.phpt to test this behavior.

The following code will break:

$lol = 'I love references';
$x = new stdClass;
$y = clone($x, ['x' => &$lol]);
var_dump($y);

Good catch. Behavior with references is not clearly specified in the RFC, but following the showcased “desugaring” of:

$cloned = clone $object;
foreach ($withProperties as $key => $value) {
    $cloned->{$key} = $value;
}
return $cloned;

would mean that unwrapping references would be the correct solution. Alternatively, we could also throw an Error that references are unsupported and defer the question to the future. What do you think @edorian?

We decided to make references an error for now and will follow-up with another RFC if someone complains / if the need arises.

FWIW, I also think unwrapping the references is the correct solution, as this is what happens for normal assignments. You can easily encounter references even if they are unique, e.g. by looping over the array by-ref that is passed to clone.

I also think unwrapping the references is the correct solution, as this is what happens for normal assignments.

Passing a reference within the array might also be an intentional attempt to make the property a reference. That's why we opted not to make a decision for now, we expect the most common use-case for clone-with to be an “array literal”.

I think it would be reasonable to at least unwrap references if they have RC=1, as these are most-likely accidental. That's generally how we treat RC=1 references. The engine is supposed to treat them as normal values, making the exact moment they are unwrapped irrelevant. E.g. var_dump() hides the fact this array contains a reference. https://3v4l.org/Pp48f#vnull

I think it would be reasonable to at least unwrap references if they have RC=1, as these are most-likely accidental.

Okay, will look into that tomorrow.

I agree with Ilija on this. I like the current code more.
You could split off the property update logic to a separate helper so that part can be reused? I'd think that either:

• A custom class may want to do some validation and call such a helper
• A custom class has entire custom logic to deal with it